Avoid json outfuncs key duplication #246
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The JSON format intends to follow the structure of the Protobuf JSON
representation of the parse nodes. The A_Const parse node is the only
one making use of the "oneof" construct (matching the union in the C code)
and was using a different representation in the generated JSON protobuf
(outputting "ival" once), vs the manual JSON output (outputting it twice).
Match the manual JSON output to the generate JSON protobuf. This changes
an output of "{"A_Const":{"ival":{"ival":123}}}" to be
"{"A_Const":{"ival":123}}" instead.
This is a breaking change in the JSON output.
|
Hmm, it looks like |
Closed
|
We (https://github.com/edgedb/edgedb) use the JSON output and are trying to upgrade to 16 but are blocked by the int constant thing. We would definitely appreciate a fix on 16 (though we can make a temporary fork with a direct fix if needed) |
|
It looks like the mismatch that was happening with protobuf wasn't actually the double key situation, but that protobuf still left out the value on |
|
I've submitted #249 |
|
Closing this for now since the integer constant issue was resolved separately, and the JSON duplication here seems best to keep here for now, to match |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reviewing #244 I noticed that we appear to incorrectly wrap
ival,sval, etc twice when outputtingA_Constnodes. That does not match the Protobuf JSON approach, which intends to have aoneofrepresented the same way as having a simple field of that type (see protocolbuffers/protobuf#3928 (comment)).This change now matches the manual JSON output to the generate JSON protobuf. This changes
an output of
{"A_Const":{"ival":{"ival":123}}}to be{"A_Const":{"ival":123}}instead.This is a breaking change in the JSON output. As such, this would warrant a minor version bump (or we could hold off until the Postgres 17 release). Note that this does not change anything in the binary Protobuf output, which is used by most wrapper libraries.